Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(agents-api): Remove auto_blob_store in favor of interceptor based system #977

Draft
wants to merge 33 commits into
base: f/switch-to-pg
Choose a base branch
from

Conversation

creatorrr
Copy link
Contributor

@creatorrr creatorrr commented Dec 19, 2024

User description

  • feat(agents-api): Add entry queries
  • refactor: Lint agents-api (CI)
  • chore: update the entyr queries
  • refactor: Lint agents-api (CI)
  • chore: inner join developer table with entry queries
  • refactor: Lint agents-api (CI)
  • wip(agents-api): Add session sql queries
  • refactor: Lint agents-api (CI)
  • chore: developers and user refactor + add test for entry queries + bug fixes
  • refactor: Lint agents-api (CI)
  • feat(agents-api): Fix tests for sessions
  • refactor: Lint agents-api (CI)
  • wip(agents-api): Entry queries
  • refactor: Lint agents-api (CI)
  • fix(agents-api): change modelname to model in BaseEntry
  • feat(agents-api): add agent queries tests
  • feat(agents-api): implement agent queries and tests
  • refactor: Lint agents-api (CI)
  • fix(agents-api): misc fixes
  • refactor: Lint agents-api (CI)
  • wip
  • refactor: Lint agents-api (CI)
  • chore: minor refactors
  • refactor: Lint agents-api (CI)
  • feat(memory-store,agents-api): Move is_leaf handling to postgres
  • fix(agents-api): fix sessions and agents queries / tests
  • refactor: Lint agents-api (CI)
  • feat(agents-api): Remove auto_blob_store in favor of interceptor based system

PR Type

Enhancement, Tests


Description

  • Replaced blob store system with new interceptor-based approach for improved storage handling
  • Added comprehensive PostgreSQL queries for session and entry management with proper error handling
  • Enhanced session models with system_template and forward_tool_calls fields
  • Improved database schema with automatic leaf node status updates and session timestamps
  • Added extensive test coverage for session and entry operations
  • Implemented developer update queries with robust error handling
  • Updated environment configurations and type specifications

Changes walkthrough 📝

Relevant files
Enhancement
8 files
Sessions.py
Add system template and tool call forwarding to session models

agents-api/agents_api/autogen/Sessions.py

  • Added system_template field to session-related models
  • Added forward_tool_calls boolean flag to session models
  • Updated model documentation with new field descriptions
  • +40/-0   
    sessions
    Implement PostgreSQL session management queries                   

    agents-api/agents_api/queries/sessions

  • Added new session management queries (create, update, delete, list,
    etc)
  • Implemented SQL queries with proper error handling and type safety
  • Added support for session metadata and participant lookups
  • entries
    Add PostgreSQL entry management queries                                   

    agents-api/agents_api/queries/entries

  • Added entry management queries for creating, listing and deleting
    entries
  • Implemented SQL queries with proper error handling
  • Added support for entry relations and history tracking
  • temporal.py
    Replace blob store with interceptor system                             

    agents-api/agents_api/clients/temporal.py

  • Replaced blob store handling with new interceptor-based system
  • Added asyncio gather for parallel processing of arguments
  • Updated imports to reflect new storage handling approach
  • +6/-3     
    count_sessions.py
    New session counting functionality                                             

    agents-api/agents_api/queries/sessions/count_sessions.py

  • Added new query module for counting sessions
  • Implemented error handling for database exceptions
  • Added proper documentation and type hints
  • +55/-0   
    000016_entry_relations.up.sql
    Improved entry relations leaf node handling                           

    memory-store/migrations/000016_entry_relations.up.sql

  • Replaced leaf node enforcement with automatic leaf status updates
  • Implemented new trigger for managing leaf node status
  • Improved database schema for entry relations
  • +19/-15 
    000015_entries.up.sql
    Enhanced session and entry table functionality                     

    memory-store/migrations/000015_entries.up.sql

  • Added 'developer' to chat_role enum
  • Added trigger for updating session timestamps
  • Implemented automatic session updated_at updates
  • +18/-2   
    models.tsp
    New session model fields and documentation                             

    typespec/sessions/models.tsp

  • Added system_template field to Session model
  • Added forward_tool_calls boolean flag
  • Updated model documentation
  • +6/-0     
    Refactor
    1 files
    protocol
    Refactor remote object handling and task protocols             

    agents-api/agents_api/common/protocol

  • Simplified RemoteObject implementation with generics
  • Removed BaseRemoteModel in favor of interceptor system
  • Updated task protocols to remove blob store dependencies
  • Tests
    1 files
    tests
    Update tests for PostgreSQL session and entry queries       

    agents-api/tests

  • Updated session and entry query tests for PostgreSQL
  • Added new test cases for session operations
  • Improved test fixtures and utilities
  • Error handling
    1 files
    update_developer.py
    Enhanced error handling in developer update query               

    agents-api/agents_api/queries/developers/update_developer.py

  • Added error handling for database exceptions using rewrap_exceptions
    decorator
  • Improved SQL query readability by using a more descriptive variable
    name
  • Added proper error handling for foreign key violations
  • +16/-8   
    Formatting
    1 files
    patch_agent.py
    Code cleanup and query naming improvements                             

    agents-api/agents_api/queries/agents/patch_agent.py

  • Removed unused imports and type variables
  • Renamed SQL query variable for better clarity
  • Improved code formatting and structure
  • +7/-13   
    Configuration changes
    1 files
    env.py
    Environment configuration updates                                               

    agents-api/agents_api/env.py

  • Modified blob store configuration logic
  • Added new query timeout configuration
  • Updated testing environment settings
  • +4/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information


    Important

    Replaced blob store with interceptor system, added PostgreSQL session and entry queries, and improved error handling and test coverage.

    • Storage System:
      • Removed auto_blob_store in favor of interceptor-based system in temporal.py and interceptors.py.
      • Replaced store_in_blob_store_if_large and load_from_blob_store_if_remote with offload_if_large and load_if_remote.
    • Database Queries:
      • Added PostgreSQL queries for session and entry management in queries/sessions and queries/entries.
      • Enhanced session models with system_template and forward_tool_calls fields in Sessions.py.
      • Improved database schema with automatic leaf node status updates and session timestamps in 000015_entries.up.sql and 000016_entry_relations.up.sql.
    • Error Handling:
      • Enhanced error handling in update_developer.py and interceptors.py.
    • Tests:
      • Added tests for session and entry queries in test_session_queries.py and test_entry_queries.py.
    • Miscellaneous:
      • Updated environment configurations in env.py.
      • Refactored query utilities in utils.py.

    This description was created by Ellipsis for ca5f4e2. It will automatically update as commits are pushed.

    Vedantsahai18 and others added 30 commits December 16, 2024 18:31
    Signed-off-by: Diwank Singh Tomer <[email protected]>
    feat(agents-api): Add entry queries
    Copy link

    gitguardian bot commented Dec 19, 2024

    ⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

    Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

    🔎 Detected hardcoded secret in your pull request
    GitGuardian id GitGuardian status Secret Commit Filename
    14144715 Triggered Generic Password c85c2fe memory-store/docker-compose.yml View secret
    🛠 Guidelines to remediate hardcoded secrets
    1. Understand the implications of revoking this secret by investigating where it is used in your code.
    2. Replace and store your secret safely. Learn here the best practices.
    3. Revoke and rotate this secret.
    4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

    To avoid such incidents in the future consider


    🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

    Copy link

    qodo-merge-pro-for-open-source bot commented Dec 19, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit ca5f4e2)

    Action: Typecheck

    Failed stage: Typecheck [❌]

    Failed test name: pytype

    Failure summary:

    The pytype check failed due to multiple issues:
    1. Missing dependency: Cannot find module pycozo
    which is required by agents_api.common.utils.cozo
    2. Type error in
    agents_api.queries.entries.create_entries: Bad return type in function add_entry_relations
    3.
    Invalid type comments in tests/test_workflow_routes.py: Stray type comments found at lines 65 and
    114

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    1178:  [9/352] check agents_api.common.utils.datetime
    1179:  [10/352] check agents_api.autogen.Executions
    1180:  [11/352] check agents_api.autogen.Entries
    1181:  [12/352] check agents_api.clients.__init__
    1182:  [13/352] check agents_api.autogen.Agents
    1183:  [14/352] check agents_api.autogen.Tasks
    1184:  [15/352] check agents_api.env
    1185:  [16/352] check agents_api.common.utils.cozo
    1186:  FAILED: /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/common/utils/cozo.pyi 
    1187:  /home/runner/work/julep/julep/agents-api/.venv/bin/python -m pytype.main --disable pyi-error --imports_info /home/runner/work/julep/julep/agents-api/.pytype/imports/agents_api.common.utils.cozo.imports --module-name agents_api.common.utils.cozo --platform linux -V 3.12 -o /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/common/utils/cozo.pyi --analyze-annotated --nofail --none-is-not-bool --quick --strict-none-binding /home/runner/work/julep/julep/agents-api/agents_api/common/utils/cozo.py
    1188:  /home/runner/work/julep/julep/agents-api/agents_api/common/utils/cozo.py:9:1: error: in <module>: Can't find module 'pycozo'. [import-error]
    1189:  from pycozo import Client
    1190:  ~~~~~~~~~~~~~~~~~~~~~~~~~
    1191:  For more details, see https://google.github.io/pytype/errors.html#import-error
    ...
    
    1201:  [26/352] check agents_api.common.utils.types
    1202:  [27/352] check agents_api.common.protocol.tasks
    1203:  [28/352] check agents_api.common.protocol.sessions
    1204:  [29/352] check agents_api.common.utils.messages
    1205:  [30/352] check agents_api.common.nlp
    1206:  [31/352] check agents_api.clients.async_s3
    1207:  [32/352] check agents_api.common.protocol.developers
    1208:  [33/352] check agents_api.queries.utils
    1209:  ERROR:pytype.matcher Invalid type: <class 'pytype.abstract.function.ParamSpecMatch'>
    ...
    
    1253:  [77/352] check agents_api.rec_sum.data
    1254:  [78/352] check agents_api.rec_sum.generate
    1255:  [79/352] check agents_api.queries.developers.update_developer
    1256:  [80/352] check agents_api.queries.developers.patch_developer
    1257:  [81/352] check agents_api.queries.entries.list_entries
    1258:  [82/352] check agents_api.queries.entries.get_history
    1259:  [83/352] check agents_api.queries.entries.delete_entries
    1260:  [84/352] check agents_api.queries.entries.create_entries
    1261:  FAILED: /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/queries/entries/create_entries.pyi 
    1262:  /home/runner/work/julep/julep/agents-api/.venv/bin/python -m pytype.main --disable pyi-error --imports_info /home/runner/work/julep/julep/agents-api/.pytype/imports/agents_api.queries.entries.create_entries.imports --module-name agents_api.queries.entries.create_entries --platform linux -V 3.12 -o /home/runner/work/julep/julep/agents-api/.pytype/pyi/agents_api/queries/entries/create_entries.pyi --analyze-annotated --nofail --none-is-not-bool --quick --strict-none-binding /home/runner/work/julep/julep/agents-api/agents_api/queries/entries/create_entries.py
    1263:  /home/runner/work/julep/julep/agents-api/agents_api/queries/entries/create_entries.py:172:5: error: in add_entry_relations: bad return type [bad-return-type]
    ...
    
    1282:  params,
    1283:  ~~~~~~~~~~~~~~~~~~~
    1284:  "fetchmany",
    1285:  ~~~~~~~~~~~~~~~~~~~~~~~~
    1286:  ),
    1287:  ~~~~~~~~~~
    1288:  ]
    1289:  ~~~~~
    1290:  For more details, see https://google.github.io/pytype/errors.html#bad-return-type
    ...
    
    1326:  [120/352] check tests.test_agent_routes
    1327:  [121/352] check agents_api.workflows.__init__
    1328:  [122/352] check agents_api.common.utils.debug
    1329:  [123/352] check tests.test_execution_workflow
    1330:  [124/352] check tests.sample_tasks.__init__
    1331:  [125/352] check agents_api.clients.worker.__init__
    1332:  [126/352] check agents_api.rec_sum.entities
    1333:  [127/352] check tests.test_workflow_routes
    1334:  /home/runner/work/julep/julep/agents-api/tests/test_workflow_routes.py:65:1: error: : Stray type comment: object [ignored-type-comment]
    1335:  #   type: object~~~~~~~~~~~~~~~
    1336:  #   type: object
    1337:  /home/runner/work/julep/julep/agents-api/tests/test_workflow_routes.py:114:1: error: : Stray type comment: object [ignored-type-comment]
    1338:  #   type: object~~~~~~~~~~~~~~~
    1339:  #   type: object
    1340:  For more details, see https://google.github.io/pytype/errors.html#ignored-type-comment
    ...
    
    1358:  [145/352] check tests.test_execution_queries
    1359:  [146/352] check agents_api.queries.users.__init__
    1360:  [147/352] check agents_api.rec_sum.__init__
    1361:  [148/352] check agents_api.dependencies.__init__
    1362:  [149/352] check agents_api.rec_sum.summarize
    1363:  [150/352] check agents_api.common.__init__
    1364:  [151/352] check tests.test_files_queries
    1365:  [152/352] check agents_api.queries.sessions.__init__
    1366:  ninja: build stopped: cannot make progress due to previous errors.
    1367:  Computing dependencies
    1368:  Generated API key since not set in the environment: 17556219117988237868194435419280
    1369:  Sentry DSN not found. Sentry will not be enabled.
    1370:  Analyzing 327 sources with 0 local dependencies
    1371:  Leaving directory '.pytype'
    1372:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    SQL injection:
    Multiple files contain SQL queries that use string formatting to build dynamic queries (e.g. agents-api/agents_api/queries/entries/list_entries.py and create_entries.py). While some parameters are properly escaped using $N placeholders, others are directly interpolated into the query string. This could potentially allow SQL injection attacks if input validation is bypassed. The code should be updated to use parameterized queries consistently for all dynamic values.

    ⚡ Recommended focus areas for review

    SQL Injection
    The SQL query uses string formatting for dynamic queries which could potentially allow SQL injection. Consider using parameterized queries consistently.

    SQL Injection
    The query string is formatted with user-provided sort_by and direction values which could enable SQL injection. Should validate these values against a whitelist.

    Performance Issue
    The blob store size check uses sys.getsizeof() which may not accurately reflect true object size for complex objects. Consider implementing a more accurate size estimation.

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Reviewed everything up to e158f3a in 1 minute and 32 seconds

    More details
    • Looked at 5962 lines of code in 91 files
    • Skipped 0 files when reviewing.
    • Skipped posting 7 drafted comments based on config settings.
    1. agents-api/agents_api/activities/embed_docs.py:14
    • Draft comment:
      Ensure that the removal of @auto_blob_store(deep=True) is compensated by the new interceptor-based system to handle large data offloading.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The removal of the auto_blob_store decorator is consistent across multiple files. This change aligns with the PR's intent to replace it with an interceptor-based system. However, the decorator's removal should be verified to ensure that the new system handles the same functionality.
    2. agents-api/agents_api/activities/execute_system.py:36
    • Draft comment:
      Verify that the new interceptor system handles the functionality previously managed by load_from_blob_store_if_remote.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The load_from_blob_store_if_remote function call is removed, which was responsible for loading data from a remote blob store. Ensure that the new interceptor system handles this functionality.
    3. agents-api/agents_api/activities/task_steps/base_evaluate.py:62
    • Draft comment:
      Ensure that the removal of @auto_blob_store(deep=True) is consistent with the new interceptor-based system for data handling.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The auto_blob_store decorator is removed from multiple functions. This change should be consistent with the new interceptor-based system to ensure data handling is still managed correctly.
    4. agents-api/agents_api/activities/task_steps/cozo_query_step.py:10
    • Draft comment:
      Ensure that the removal of @auto_blob_store(deep=True) is consistent with the new interceptor-based system for data handling.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The auto_blob_store decorator is removed from multiple functions. This change should be consistent with the new interceptor-based system to ensure data handling is still managed correctly.
    5. agents-api/agents_api/activities/task_steps/evaluate_step.py:10
    • Draft comment:
      Ensure that the removal of @auto_blob_store(deep=True) is consistent with the new interceptor-based system for data handling.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The auto_blob_store decorator is removed from multiple functions. This change should be consistent with the new interceptor-based system to ensure data handling is still managed correctly.
    6. agents-api/agents_api/activities/task_steps/for_each_step.py:11
    • Draft comment:
      Ensure that the removal of @auto_blob_store(deep=True) is consistent with the new interceptor-based system for data handling.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The auto_blob_store decorator is removed from multiple functions. This change should be consistent with the new interceptor-based system to ensure data handling is still managed correctly.
    7. agents-api/agents_api/activities/task_steps/get_value_step.py:11
    • Draft comment:
      Ensure that the removal of @auto_blob_store(deep=True) is consistent with the new interceptor-based system for data handling.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The auto_blob_store decorator is removed from multiple functions. This change should be consistent with the new interceptor-based system to ensure data handling is still managed correctly.

    Workflow ID: wflow_AjDxfoccjl58OUog


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix SQL parameter count mismatch in entry relations query

    The SQL query for entry relations has an incorrect number of parameters. The VALUES
    clause has 5 placeholders but only 4 parameters are provided in the query string.

    agents-api/agents_api/queries/entries/create_entries.py [43-50]

     entry_relation_query = """
     INSERT INTO entry_relations (
         session_id,
         head,
         relation,
    -    tail,
    -) VALUES ($1, $2, $3, $4, $5)
    +    tail
    +) VALUES ($1, $2, $3, $4)
     RETURNING *;
     """
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Critical bug fix: The SQL query has 5 placeholders ($1-$5) but only 4 columns in the VALUES clause, which would cause a database error. The fix removes the trailing comma and extra placeholder.

    10
    Add timeout handling for remote object loading to prevent workflow hangs

    Add timeout handling for remote object loading operations to prevent indefinite
    hangs.

    agents-api/agents_api/common/interceptors.py [52-56]

     async def load_if_remote[T](arg: T | RemoteObject[T]) -> T:
         if use_blob_store_for_temporal and isinstance(arg, RemoteObject):
    -        return await arg.load()
    +        try:
    +            return await asyncio.wait_for(arg.load(), timeout=30.0)
    +        except asyncio.TimeoutError:
    +            raise ApplicationError("Remote object load timeout", non_retryable=True)
         return arg
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding timeout handling is critical for system stability as it prevents indefinite hangs in workflows when remote objects fail to load. The suggestion provides a concrete timeout value and proper error handling.

    9
    Fix incorrect error message and status code for unique constraint violations

    The error message in the UniqueViolationError handler incorrectly states "The
    specified developer does not exist" when it should indicate a duplicate developer
    error. Update the error message to accurately reflect the unique constraint
    violation.

    agents-api/agents_api/queries/developers/create_developer.py [39-43]

     asyncpg.UniqueViolationError: partialclass(
         HTTPException,
    -    status_code=404,
    -    detail="The specified developer does not exist.",
    +    status_code=409,
    +    detail="A developer with this ID or email already exists.",
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion fixes a critical issue where the error handling provides misleading information and incorrect HTTP status code (404 instead of 409) for unique constraint violations, which could confuse API clients and make debugging harder.

    9
    Add error handling for blob store operations to prevent silent failures

    Add error handling for the blob store operations to handle potential
    storage/retrieval failures gracefully.

    agents-api/agents_api/common/interceptors.py [59-63]

     async def offload_if_large[T](result: T) -> T:
    -    if use_blob_store_for_temporal and is_too_large(result):
    -        return await RemoteObject.from_value(result)
    -    return result
    +    try:
    +        if use_blob_store_for_temporal and is_too_large(result):
    +            return await RemoteObject.from_value(result)
    +        return result
    +    except Exception as e:
    +        raise ApplicationError(f"Failed to offload large result: {str(e)}", non_retryable=True)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for blob store operations is crucial for system reliability, as silent failures in storage operations could lead to data loss or corruption. The suggestion properly wraps errors in ApplicationError with non-retryable flag.

    8
    Remove incorrect database error handler for read operation

    The UniqueViolationError handler is incorrectly used for a GET operation - this
    error cannot occur during a SELECT query. Remove this unnecessary error handler.

    agents-api/agents_api/queries/users/get_user.py [34-38]

    -asyncpg.UniqueViolationError: partialclass(
    +asyncpg.NoDataFoundError: partialclass(
         HTTPException,
         status_code=404,
         detail="The specified user does not exist.",
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion identifies and fixes an incorrect error handler that cannot occur during SELECT operations, replacing it with the appropriate NoDataFoundError handler for better error handling accuracy.

    8
    Prevent unintentional NULL overwrites in PATCH operations

    The SQL query uses COALESCE for all fields which could unintentionally overwrite
    existing values with NULL when fields are not provided in the patch request. Use a
    dynamic SET clause that only updates provided fields.

    agents-api/agents_api/queries/sessions/patch_session.py [17-25]

     SET
    -    situation = COALESCE($3, situation),
    -    system_template = COALESCE($4, system_template),
    +    situation = CASE WHEN $3 IS NOT NULL THEN $3 ELSE situation END,
    +    system_template = CASE WHEN $4 IS NOT NULL THEN $4 ELSE system_template END,
         metadata = sessions.metadata || $5,
    -    render_templates = COALESCE($6, render_templates),
    -    token_budget = COALESCE($7, token_budget),
    -    context_overflow = COALESCE($8, context_overflow),
    -    forward_tool_calls = COALESCE($9, forward_tool_calls),
    +    render_templates = CASE WHEN $6 IS NOT NULL THEN $6 ELSE render_templates END,
    +    token_budget = CASE WHEN $7 IS NOT NULL THEN $7 ELSE token_budget END,
    +    context_overflow = CASE WHEN $8 IS NOT NULL THEN $8 ELSE context_overflow END,
    +    forward_tool_calls = CASE WHEN $9 IS NOT NULL THEN $9 ELSE forward_tool_calls END,
         recall_options = sessions.recall_options || $10
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the robustness of the PATCH operation by preventing unintentional NULL overwrites of existing values when fields are not provided in the request, using CASE statements instead of COALESCE.

    7
    Add type validation when loading remote objects to prevent runtime type errors

    Add type validation for the input arguments before loading from blob store to ensure
    type safety. The current implementation assumes all args can be remote objects
    without validation.

    agents-api/agents_api/common/interceptors.py [79-80]

     if use_blob_store_for_temporal:
    -    input.args = await asyncio.gather(*[load_if_remote(arg) for arg in args])
    +    input.args = await asyncio.gather(*[
    +        load_if_remote(arg) if isinstance(arg, (RemoteObject, T)) else arg 
    +        for arg in args
    +    ])
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion adds type validation to prevent potential runtime errors when loading remote objects, improving type safety. However, the impact is moderate since the existing code would likely handle most cases correctly.

    5

    Signed-off-by: Diwank Singh Tomer <[email protected]>
    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    👍 Looks good to me! Incremental review on ca5f4e2 in 50 seconds

    More details
    • Looked at 53 lines of code in 4 files
    • Skipped 0 files when reviewing.
    • Skipped posting 3 drafted comments based on config settings.
    1. agents-api/tests/fixtures.py:3
    • Draft comment:
      The import of time is unnecessary as it is not used in this file. Consider removing it to clean up the code.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The import of time in agents-api/tests/fixtures.py is unnecessary as it is not used anywhere in the file. Removing unused imports is a good practice to keep the code clean and efficient.
    2. agents-api/agents_api/queries/sessions/create_session.py:11
    • Draft comment:
      The import of ResourceCreatedResponse is unnecessary as it is not used in this file. Consider removing it to clean up the code.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The import of ResourceCreatedResponse in agents-api/agents_api/queries/sessions/create_session.py is unnecessary as it is not used anywhere in the file. Removing unused imports is a good practice to keep the code clean and efficient.
    3. agents-api/tests/test_session_queries.py:13
    • Draft comment:
      The import of ResourceCreatedResponse is unnecessary as it is not used in this file. Consider removing it to clean up the code.
    • Reason this comment was not posted:
      Confidence changes required: 50%
      The import of ResourceCreatedResponse in agents-api/tests/test_session_queries.py is unnecessary as it is not used anywhere in the file. Removing unused imports is a good practice to keep the code clean and efficient.

    Workflow ID: wflow_4DsKSc8XWp0obflW


    You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

    @creatorrr creatorrr marked this pull request as draft December 21, 2024 08:25
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants